Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Codeframe fixes #5094

Merged
merged 8 commits into from
Dec 17, 2017
Merged

Codeframe fixes #5094

merged 8 commits into from
Dec 17, 2017

Conversation

SimenB
Copy link
Member

@SimenB SimenB commented Dec 16, 2017

Summary
Fixes a couple of issues with the new code frame.

In particular it fixes async assertions and having an expect in another file than the test file.

I would like to fix async assertions to have a codeframe, but that can be done separately (currently it's not in the stack trace at all).

Test plan
Tests added


10 |
11 | module.exports = (one: any, two: any) => {
> 12 | expect(one).toEqual(two);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potentially we can walk down the stack until we find the test file. But I think this makes more sense

@SimenB
Copy link
Member Author

SimenB commented Dec 17, 2017

The failing test is passing on my machine (mac, [email protected])... Ideas?

Command I run: CI=true TRAVIS=true ./jest -i --no-cache, all green

Also fix integration test which worked differently depending on if the
test had been run before or not
@SimenB
Copy link
Member Author

SimenB commented Dec 17, 2017

Found the bug, hopefully green CI now

@codecov-io
Copy link

codecov-io commented Dec 17, 2017

Codecov Report

Merging #5094 into master will increase coverage by 0.07%.
The diff coverage is 91.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5094      +/-   ##
==========================================
+ Coverage   60.58%   60.66%   +0.07%     
==========================================
  Files         201      201              
  Lines        6686     6694       +8     
  Branches        4        4              
==========================================
+ Hits         4051     4061      +10     
+ Misses       2635     2633       -2
Impacted Files Coverage Δ
packages/jest-message-util/src/index.js 84.48% <91.66%> (+3%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update df92340...620787c. Read the comment docs.

.filter(
line =>
!line.includes(`${path.sep}node_modules${path.sep}`) &&
!line.includes(`${path.sep}expect${path.sep}build${path.sep}`),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this second one is only needed in jest's own tests, as we resolve the symlink properly. In normal cases only node_modules is needed

@cpojer cpojer merged commit fb10f7a into jestjs:master Dec 17, 2017
@cpojer
Copy link
Member

cpojer commented Dec 17, 2017

I love it. Thanks for working on this feature.

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants